Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add slicer plugin #6857

Merged
merged 101 commits into from
Nov 11, 2023
Merged

Add slicer plugin #6857

merged 101 commits into from
Nov 11, 2023

Conversation

DanielKauss
Copy link
Contributor

@DanielKauss DanielKauss commented Sep 9, 2023

This PR adds a very basic sample slicer. The main features are:

  • Ability to cut samples into slices
  • Time scale samples to fit bpm, without pitch shifting
  • Copy midi pattern to clipboard
  • Simple slice editor
  • Basic slice and bpm detection

268527804-eb4398d2-9514-4ad2-8d7f-0a1a72316900

The UI is still in the works at the moment.

I have no experience in music production and wanted to start with LMMS, but noticed it doesn't include a slicer, so I tried making one. I don't have a lot of samples to test it so feedback would be appreciated.

I am sorry if this is inadequate for the main repo, this is my first time contributing. But if it is something worthwhile I will continue working on it.

@Rossmaxx
Copy link
Contributor

Rossmaxx commented Sep 9, 2023

I appreciate the functionality but this is not the way to add slicing and tempo stretching. These stuff should either go into afp (slicing is already partially implemented there) or sample tracks (where they make sense).

@Rossmaxx
Copy link
Contributor

Rossmaxx commented Sep 9, 2023

Also, forgot to mention that slicing is already possible in the nightly using the knife tool.

@RoxasKH
Copy link
Contributor

RoxasKH commented Sep 9, 2023

I disagree, i wouldn't see why it should go into AFP at all, considering a sampler and a sample slicer serve different purposes and there wouldn't be a way to merge both flawlessly imo.
This is not samples stretching or some basic samples manipulation, nor it's a multisampler (even if it shares some principles).

About slicing on samples tracks, that's already been implemented, but It once again server a different purpose imo.

This is a slicer plugin like ninjas2 and it would really help workflow with sliced vocal chops or drums beat like amen break and such, something where using samples tracks would be way more inefficient.

I do appreciate the effort and imo is still something to consider to have in LMMS.

About the GUI i could try sketching something up if i get some free time, or anyway you could probably find someone else to help about it on LMMS discord as well, unless you plan to design it yourself.

@DanielKauss
Copy link
Contributor Author

I feel like afp would not be the correct place to add slicing, since its purpose is more the playback of a single file. Also it uses the piano roll to pitch shift already, so you would not be able to use it to play different slices. This and afp have totally separate use cases.
As for time shifting, the more places have it the better obviously.
I didn't know of the slicing in the nightly, but I primarily made this referencing other DAWs, which have a dedicated slicer plugin. Even with the note slicing, you cant really see the waveform and make the slices yourself, so the process would be pretty tedious (I haven't actually tested it, just my guess).
If you can replicate this functionality only using the afp and sample tracks, I would be happy if you could give some starting points to actually implement it, but I feel like this is something totally separate.
Sorry for any mistakes or assumptions I made, like I said, I know nothing about music production.

@DanielKauss
Copy link
Contributor Author

About the GUI i could try sketching something up if i get some free time, or anyway you could probably find someone else to help about it on LMMS discord as well, unless you plan to design it yourself.

Any help would be appreciated, I am not very good at UI design, but of course only if this is something acceptable to include in LMMS.

@consolegrl
Copy link
Contributor

consolegrl commented Sep 9, 2023

I just want to say that I was planning on creating a plugin like this, well done! I have some old FL projects I can't import because LMMS has no Beat/Fruity Slicer.

AFP kinda seems like the wrong place for slicing, bc you would have to check some kind of keyboard tracking switch to have each note map to a slice or to pitch shift the sample, but not both at once.

I will be helping with this when I get time because this is a killer feature. +100 you win the Internet.

@Rossmaxx
Copy link
Contributor

Rossmaxx commented Sep 9, 2023

Sorry for the confusion, i got confused between sample slicing and chopping. Since i haven't used slicing in any way till now, i can't say anything. Whatever i said was regarding chopping.

@sakertooth
Copy link
Contributor

I began working on this awhile back, although I never really continued with it. It would be nice if someone could pick it up because I'm focusing on more pressing issues in the codebase. I attached an image below of what I had, just a simple interface, but uses Qt layouts and a larger window. The latter is important as it allows for more freedom moving the slice positions. For a larger window, I just had a "Show GUI" button that would open the larger slicer plugin.

image
image

Having a larger window is just part of my vision for the plugin. Someone else may have a different vision, but a larger window would probably be nice all round.

A lot could go into this. Depending on how far you want to stretch, users may request ability to timestretch and pitchshift specific slices (we would probably have to pull in another library for this), ability to choose cues like in Serato Sample (or onsets like in Ninjas2). You don't have to worry about adding those features, they can always be added later, but I think having a spacious interface and ability to add slice markers and move them around is enough for now.

@DanielKauss
Copy link
Contributor Author

DanielKauss commented Sep 9, 2023

I just want to say that I was planning on creating a plugin like this, well done! I have some old FL projects I can't import because LMMS has no Beat/Fruity Slicer.

good to know that I'm not alone in wanting this, feel free to help however you want.

Sorry for the confusion, i got confused between sample slicing and chopping. Since i haven't used slicing in any way till now, i can't say anything. Whatever i said was regarding chopping.

ok, no problem.

I began working on this awhile back, although I never really continued with it. It would be nice if someone could pick it up because I'm focusing on more pressing issues in the codebase. I attached an image below of what I had, just a simple interface, but uses Qt layouts and a larger window. The latter is important as it allows for more freedom moving the slice positions. For a larger window, I just had a "Show GUI" button that would open the larger slicer plugin.

I wanted to avoid a larger window at the beginning to fit in with the other plugins, so I just added a seeker to place slices with precision. However, If I want to add any more functionality like effects or whatever I will do this. As for needing another library to time and pitch shift, the code for that is already implemented, although pitch shifting doesn't quite work yet.

Since this does seem like something that could be added, I will continue working on it.

@zonkmachine
Copy link
Member

I primarily made this referencing other DAWs, which have a dedicated slicer plugin.

I've never worked with slicers before. The closest I've been is sitting next to someone working with Recycle.
After a quick test I can conclude that the plugin works, is easy to get started with and work with. 👍

Copy link
Member

@zonkmachine zonkmachine left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just dropping some general remarks on coding style.

plugins/SlicerT/SlicerT.cpp Outdated Show resolved Hide resolved
plugins/SlicerT/SlicerT.cpp Outdated Show resolved Hide resolved
@DanielKauss
Copy link
Contributor Author

Could the naming of copyMidi.png and resetSlices.png be changed to use snake case? That seems like the naming pattern we use for our pixmaps.

Done. Also renamed icon to logo.

Copy link
Contributor

@sakertooth sakertooth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving for merge.

@sakertooth sakertooth changed the title Simple slicer plugin Add slicer plugin Nov 11, 2023
@messmerd
Copy link
Member

messmerd commented Nov 11, 2023

Is there any chance that in the future (or even in this PR) we'd want to assign a different purpose to keys outside the range used by the individual slices? For example, maybe we would want keys below the root key (and below the key that allows the entire sample to play) to play slices in reverse or on loop or something?

If we merge now then in the future decide that we want to give all those wasted keys a new purpose, it would require an upgrade routine. But if we disallow those notes from being played right now, we wouldn't need an upgrade routine in the future.

@messmerd
Copy link
Member

Maybe the base note (A4 by default) could be the note that loops the entire sample (instead of being the 1st slice), then notes above that would play the slices like normal, and notes below that could have a special purpose in the future. Notes outside the range would play nothing.

@DanielKauss
Copy link
Contributor Author

Is there any chance that in the future (or even in this PR) we'd want to assign a different purpose to keys outside the range used by the individual slices? For example, maybe we would want keys below the root key (and below the key that allows the entire sample to play) to play slices in reverse or on loop or something?

Not in this PR, but yes, maybe in the future I can make the bottom keys play in reverse or something similar. I mostly want to get this merged soon, and in the future I'll probably come back to expand the plugin a bit more.

Maybe the base note (A4 by default) could be the note that loops the entire sample (instead of being the 1st slice), then notes above that would play the slices like normal, and notes below that could have a special purpose in the future. Notes outside the range would play nothing.

Done, now only the base note plays the full sample.

Copy link
Member

@messmerd messmerd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code looks good, and I haven't had any issues while testing.

@messmerd
Copy link
Member

Looks like there might be a memory leak.. I'll look into this soon.

Copy link
Member

@messmerd messmerd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Found the source of the memory leak

plugins/SlicerT/SlicerT.cpp Outdated Show resolved Hide resolved
plugins/SlicerT/SlicerT.cpp Show resolved Hide resolved
plugins/SlicerT/SlicerT.h Show resolved Hide resolved
DanielKauss and others added 2 commits November 11, 2023 23:40
@messmerd
Copy link
Member

@DanielKauss If you're satisfied with the PR, I can go ahead and merge.

@DanielKauss
Copy link
Contributor Author

@DanielKauss If you're satisfied with the PR, I can go ahead and merge.

Fine by me!

@messmerd messmerd merged commit c779521 into LMMS:master Nov 11, 2023
9 checks passed
@messmerd
Copy link
Member

Great work! Thank you for contributing!

@DanielKauss
Copy link
Contributor Author

Great work! Thank you for contributing!

Thanks! And also thank you to everyone who helped!

@gnlscience
Copy link

This ish gonna be killa. sony acid had this feature. as a beatmaker , thanks for this

@zonkmachine
Copy link
Member

@DanielKauss Awesome work!...

I've been troubleshooting SlicerT since I've had issues with noise after merge into master. One issue I found is that we do division with 0 when fadeOut is 0.

With -DWANT_DEBUG_FPE

Thread 37 "AudioEngine::fi" received signal SIGFPE, Arithmetic exception.
[Switching to Thread 0x7fff79494640 (LWP 90724)]
0x00007fff4283d138 in lmms::SlicerT::playNote (this=0x7ffff27f0680, handle=0x7ffff2750e60, workingBuffer=0x7fff41331880) at /home/zonkmachine/builds/lmms/plugins/SlicerT/SlicerT.cpp:138
138				float fadeValue = static_cast<float>(noteFramesLeft - i) / fadeOutFrames;

@zonkmachine
Copy link
Member

zonkmachine commented Nov 14, 2023

I've had issues with noise after merge into master.

I believe this is the PR that creates noise. The line below has been removed from the other native plugins since #6867

instrumentTrack()->processAudioBuffer(workingBuffer, frames + offset, handle);

@DanielKauss
Copy link
Contributor Author

I've been troubleshooting SlicerT since I've had issues with noise after merge into master. One issue I found is that we do division with 0 when fadeOut is 0.

I believe this is the PR that creates noise. The line below has been removed from the other native plugins since #6867

Thanks for finding these issues. I'll open another PR later that fixes this. I'll also add reversed slices, since it seems like a few people want that.

@mirk0dex
Copy link
Contributor

This is by far the sleekest looking inbuilt plugin we have. Props to the creator.

@DanielKauss
Copy link
Contributor Author

@zonkmachine I opened up the bug fix PR here #6992, with additional support for reversed slices

This is by far the sleekest looking inbuilt plugin we have. Props to the creator.

I agree, @RoxasKH did an amazing job!

@Rossmaxx Rossmaxx mentioned this pull request Nov 19, 2023
@Veratil Veratil mentioned this pull request Feb 15, 2024
@chiaravalle
Copy link

In my mind, a slice has 3 components:

An ID
A pitch
A length
The ID is used to refer to a slice, the pitch to play the slice at the desired pitch, the length will loop the slice ID at the Slice ID's loop points until the end of its associated note off event.
This way, a slicer can also be altered/remixed easily

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.